feat(pt_expt): auto-select O(N) NeighborGraph builder by default - #5903
feat(pt_expt): auto-select O(N) NeighborGraph builder by default#5903Shaurya2k06 wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughChangesThe inference path now uses a shared resolver to select NeighborGraph auto selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DeepEval
participant resolve_auto_graph_builder
participant nv
participant vesin
participant dense
DeepEval->>resolve_auto_graph_builder: Resolve auto builder for DEVICE
resolve_auto_graph_builder->>nv: Check CUDA availability
resolve_auto_graph_builder->>vesin: Check Vesin availability
resolve_auto_graph_builder-->>DeepEval: Return nv, vesin, or dense
DeepEval->>dense: Use dense when no preferred backend is available
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/tests/pt_expt/model/test_graph_builder_dispatch.py (1)
146-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert dispatch, not only parity.
All backends are intentionally value-equivalent, so this passes if
None/"auto"incorrectly falls back todense. Mock or spy on the resolver/concrete builder and assert that the resolved backend is invoked.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/tests/pt_expt/model/test_graph_builder_dispatch.py` around lines 146 - 158, Update test_none_and_auto_match_resolved_builder to spy on or mock the resolver/concrete graph builder, then assert that the backend resolved for None and "auto" is actually invoked. Retain the existing output-parity assertions, but ensure the test fails if either input silently falls back to dense.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deepmd/pt_expt/utils/vesin_graph_builder.py`:
- Around line 11-15: Update the documentation describing the shared
resolve_auto_graph_builder default ladder to state that vesin is selected for
CPU or CUDA fallback only when vesin.torch is importable; otherwise document
that dense is selected.
---
Nitpick comments:
In `@source/tests/pt_expt/model/test_graph_builder_dispatch.py`:
- Around line 146-158: Update test_none_and_auto_match_resolved_builder to spy
on or mock the resolver/concrete graph builder, then assert that the backend
resolved for None and "auto" is actually invoked. Retain the existing
output-parity assertions, but ensure the test fails if either input silently
falls back to dense.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d02887c0-8595-4f4e-a31f-c9bc7c3749a5
📒 Files selected for processing (9)
deepmd/pt_expt/infer/deep_eval.pydeepmd/pt_expt/model/make_model.pydeepmd/pt_expt/train/training.pydeepmd/pt_expt/utils/neighbor_graph_method.pydeepmd/pt_expt/utils/nv_graph_builder.pydeepmd/pt_expt/utils/vesin_graph_builder.pysource/tests/pt_expt/infer/test_graph_deepeval.pysource/tests/pt_expt/model/test_graph_builder_dispatch.pysource/tests/pt_expt/utils/test_neighbor_graph_method.py
987d303 to
409bb04
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5903 +/- ##
==========================================
- Coverage 79.53% 79.28% -0.25%
==========================================
Files 1075 1075
Lines 126134 126142 +8
Branches 4592 4592
==========================================
- Hits 100315 100011 -304
- Misses 24164 24478 +314
+ Partials 1655 1653 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
409bb04 to
451f50d
Compare
|
Rebased onto current master after #5912 / #5913 landed the training/eager auto path. What changed in this update
CodeQL empty- |
Training keeps CPU on dense (vesin loops frames). Inference auto now shares resolve_auto_graph_builder: CUDA nv→vesin→dense, CPU vesin→dense. Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
451f50d to
01253d9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
deepmd/pt_expt/utils/graph_builder.py:33
- Docstring names the CUDA dependency as
nvalchemiops, but the rest of the module (warnings/errors) refers to the pip packagenvalchemi-toolkit-ops. Using the installable package name here avoids confusion.
* CUDA: ``nv`` if ``nvalchemiops`` is importable, else ``vesin`` if
``vesin.torch`` is importable, else ``dense``.
deepmd/pt_expt/utils/graph_builder.py:56
- When
neighbor_graph_method='auto'falls back todenseon CUDA due to missing optional deps, the warning doesn’t tell users how to enable the faster backends (unlike the training-path warning below). Adding install hints makes the message actionable.
log.warning(
"nvalchemi-toolkit-ops and vesin[torch] are unavailable; falling "
"back from neighbor_graph_method='auto' to the dense graph builder."
)
deepmd/pt_expt/utils/graph_builder.py:31
- PR description/issue state that the model-level/training default should follow the same CPU/CUDA ladder as inference (CPU preferring vesin when available), but the code here explicitly documents (and
resolve_neighbor_graph_methodenforces) a separate training policy that keeps CPU ondense. This means the implementation doesn’t match the stated acceptance criteria unless the PR description/issue closure is adjusted.
This issue also appears in the following locations of the same file:
- line 32
- line 53
Single owner of the inference / DeepEval auto ladder (training uses
:func:`resolve_neighbor_graph_method`, which keeps CPU on ``dense`` because
vesin loops frames in Python and is not safe as a multi-frame training
default):
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deepmd/pt_expt/utils/graph_builder.py (1)
58-60: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider logging the CPU dense fallback for parity with the CUDA branch.
The CUDA branch logs a warning when it falls back to
dense(Lines 53-56). The CPU branch falls back todensesilently at Line 60. Dense is the O(N²) carry-all builder; silently downgrading to it on CPU (e.g.vesin[torch]not installed) can cause an unexplained performance regression on large systems, with no diagnostic for the user to act on.Add a similar
log.warning(orlog.info) call before returning"dense"on the CPU path, mentioning how to installvesin[torch]. If you make this change, updatetest_auto_resolution's("cpu", False, False, "dense", False)case insource/tests/pt_expt/infer/test_deep_eval_pt_checkpoint.pytowarns=True.♻️ Proposed fix
if is_vesin_torch_available(): return "vesin" + log.warning( + "vesin[torch] is unavailable; falling back from " + "neighbor_graph_method='auto' to the dense graph builder on CPU. " + "Install it with `pip install vesin[torch]` to enable the O(N) " + "vesin graph builder." + ) return "dense"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deepmd/pt_expt/utils/graph_builder.py` around lines 58 - 60, Update the CPU fallback in the graph-builder backend resolution function to log a warning or info message before returning "dense", explicitly mentioning installation of vesin[torch]. Also update the test_auto_resolution case for ("cpu", False, False, "dense") so it expects a warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@deepmd/pt_expt/utils/graph_builder.py`:
- Around line 58-60: Update the CPU fallback in the graph-builder backend
resolution function to log a warning or info message before returning "dense",
explicitly mentioning installation of vesin[torch]. Also update the
test_auto_resolution case for ("cpu", False, False, "dense") so it expects a
warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46027a52-d036-4bf6-9812-cd1fa7c5ac10
📒 Files selected for processing (4)
deepmd/pt_expt/infer/deep_eval.pydeepmd/pt_expt/utils/graph_builder.pydeepmd/pt_expt/utils/vesin_graph_builder.pysource/tests/pt_expt/infer/test_deep_eval_pt_checkpoint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/pt_expt/utils/vesin_graph_builder.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
deepmd/pt_expt/utils/graph_builder.py:31
- The new helper documents/supports
neighbor_graph_method="auto", but the core builder dispatch (build_neighbor_graph_for_method) still only accepts concrete methods. Outside ofDeepEval._resolve_neighbor_graph_method, passingneighbor_graph_method="auto"into a pt_expt model graph path would still raise aValueErrorfrom the builder dispatcher. Either wire auto-resolution into the model/dispatcher, or clarify here that callers must resolve "auto" before dispatching.
"""Resolve ``neighbor_graph_method="auto"`` to a concrete inference builder.
Single owner of the inference / DeepEval auto ladder (training uses
:func:`resolve_neighbor_graph_method`, which keeps CPU on ``dense`` because
vesin loops frames in Python and is not safe as a multi-frame training
deepmd/pt_expt/utils/vesin_graph_builder.py:16
- This module docstring reads as if
neighbor_graph_method="auto"is a general pt_expt model option, but currently the only in-tree resolver for "auto" is DeepEval (and the graph builder dispatcher itself rejects "auto"). Consider clarifying that "auto" here refers to DeepEval/inference resolution so users don’t try passing "auto" directly into modelneighbor_graph_methodand hit a runtimeValueError.
for ``nf == 1`` inference and CPU use. Inference ``neighbor_graph_method="auto"``
(:func:`~deepmd.pt_expt.utils.graph_builder.resolve_auto_graph_builder`) selects
vesin only when ``vesin.torch`` is importable (CPU always; CUDA only when ``nv``
is unavailable); otherwise it falls back to ``dense``. Training auto keeps CPU
on ``dense`` and never selects vesin. Prefer ``nv`` (:mod:`.nv_graph_builder`)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
deepmd/pt_expt/utils/graph_builder.py:56
- The warning emitted when falling back from neighbor_graph_method='auto' on CUDA is no longer actionable: it names missing optional deps but doesn't tell users how to install/enable them. Elsewhere in this module the training fallback warning and ImportErrors include a
pip install ...hint, so this should too.
log.warning(
"nvalchemi-toolkit-ops and vesin[torch] are unavailable; falling "
"back from neighbor_graph_method='auto' to the dense graph builder."
)
deepmd/pt_expt/utils/graph_builder.py:30
- The PR description/linked issue state that the model-level default (and compiled training eager _forward_graph) should use this shared auto-selection ladder. In the current code, resolve_auto_graph_builder is only called from pt_expt DeepEval (and tests), and the model path still uses its existing default-flip logic in pt_expt/model/make_model.py. Either extend the call sites as described, or adjust the PR description to match the actual scope.
def resolve_auto_graph_builder(
device: torch.device | str,
) -> str:
"""Resolve ``neighbor_graph_method="auto"`` to a concrete inference builder.
Single owner of the inference / DeepEval auto ladder (training uses
:func:`resolve_neighbor_graph_method`, which keeps CPU on ``dense`` because
vesin loops frames in Python and is not safe as a multi-frame training
default):
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Thanks for picking this up. Before the objections: the part I asked for on #5912 and did not get there, you have delivered here. The five ladder branches are driven deterministically by patching is_nv_available / is_vesin_torch_available / the device rather than recomputing the cascade inside the assertion, so test_resolve_auto_graph_builder_ladder genuinely fails without the change instead of comparing the code to a copy of itself. Extracting the ladder into one function so DeepEval and training stop carrying private copies is also the right direction.
My problem is with what the remaining commit actually changes, and with the description of it. Four comments inline.
The short version: the default flip this PR is named for already shipped -- #5912 landed DeepEval's auto ladder and #5913 landed training's, and both are ancestors of this branch. What is left is one substantive commit, and its real content is that vesin becomes an automatically selected builder. That is precisely the rung that was removed from #5912 in response to review before it merged, and the reasoning that removed it has not changed.
To be clear about what does not conflict: #5913 owns resolve_neighbor_graph_method for training and this adds a separate inference-only function, so the two are complementary and the branch merges cleanly. I am not asking you to rebase or coordinate with it.
| "back from neighbor_graph_method='auto' to the dense graph builder." | ||
| ) | ||
| return "dense" | ||
| if is_vesin_torch_available(): |
There was a problem hiding this comment.
This is the change the PR really makes, and it re-adds what #5912 removed.
On #5912 I raised this against an identical ladder:
This branch puts vesin on the default path, which contradicts the builder's own documented contract. [...] After this change, any environment with
vesin.torchinstalled and without CUDA + nvalchemiops gets that per-frame Python loop by default. That includes runs underauto_batch_size, which deliberately batches many frames into a single_eval_model_graphcall - exactly where the vectorised dense builder handles all frames at once and the vesin loop is worst. [...] Either preferdensewhennframes > 1, or update the contract to say vesin is now a default and re-examine whether the per-frame loop is acceptable there.
That rung was dropped and #5912 merged without it. This commit restores it and takes the second half of that either/or -- the module docstring is rewritten -- but not the first: there is no nframes gate, and no measurement re-examining the loop.
The reason I do not think the docstring rewrite settles it is that the repository already has an answer to this exact question, and it goes the other way. _select_neighbor_builder picks between the same two builders:
if device.type == "cpu" and nf == 1 and is_vesin_torch_available():
return VesinNeighborList()with the rationale stated directly above it: "Every other case -- any CUDA input or any multi-frame batch -- uses nvalchemiops, whose batched kernel amortizes the launch cost across frames." It takes nf, and it is called per forward. This resolver takes only device and is called once from _setup_neighbor_backend inside DeepEval.__init__, before any frame count exists -- so it cannot express that policy even if we wanted it to. That is a structural difference, not a parameter we forgot to thread through.
Three things make the exposure wider than it looks. vesin[torch] is an unconditional dependency of the torch extra, so this is the CPU default for essentially every pt user rather than an opt-in for people who installed something extra. auto_batch_size defaults to True, and _eval_model_graph receives whole batches from execute_all, so multi-frame is the normal case for dp test and dp model-devi, not the exception. And the builder's own scope note puts the cost at "~1 ms/frame call overhead". For a small system over many frames the arithmetic points at a slowdown, which inverts the PR's stated goal.
What would resolve it, in order of preference: move the resolution to call time and gate on nf == 1 for vesin, matching _select_neighbor_builder; or keep construction-time resolution and drop the vesin rung, leaving it explicit opt-in as #5912 concluded; or keep it and post a benchmark over a realistic dp test batch showing dense is not faster. Any of the three is fine by me -- what I do not want is the decision being reversed silently.
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def resolve_auto_graph_builder( |
There was a problem hiding this comment.
The summary describes two changes that are not in this diff.
It says the PR will "Flip the pt_expt model-level default (None / "auto") from hard-coded "dense" to that ladder" and "use the same helper in ... compiled training's eager _forward_graph". Neither deepmd/pt_expt/model/make_model.py nor deepmd/pt_expt/train/training.py appears in the changed files, and both still resolve the same way they did before:
make_model.py_resolve_graph_methodstill ends atgetattr(self, "neighbor_graph_method", "dense"), so a model driven directly still defaults todenseon every device;training.pystill imports onlyresolve_neighbor_graph_method, and_forward_graphstill readsgetattr(_model, "neighbor_graph_method", "dense").
build_neighbor_graph_for_method also has no "auto" branch -- it raises ValueError on anything it does not recognise -- so "auto" could not reach it even if the model default did produce it.
I think this is stale text rather than a missing change: those two flips landed in #5912 and #5913, which are already ancestors of this branch. The "Why existing tests missed this" paragraph has the same problem, since it describes fixing compiled training's hardcoded dense. Worth rewriting the body to what the commit does -- add vesin to the inference auto ladder and extract the shared helper -- because as written a reviewer would look for a model-level behaviour change that is not here, and a bisect later would be misled about where the flip came from.
| def resolve_auto_graph_builder( | ||
| device: torch.device | str, | ||
| ) -> str: | ||
| """Resolve ``neighbor_graph_method="auto"`` to a concrete inference builder. |
There was a problem hiding this comment.
Missing the numpydoc sections the rest of this module uses.
This is a public function -- no leading underscore, imported by deep_eval.py -- and its sibling resolve_neighbor_graph_method a few lines below carries full Parameters / Returns / Raises blocks. This one documents the ladder in prose and a bullet list with no Parameters for device and no Returns for the str.
_select_neighbor_builder in deepmd/pt/model/model/sezm_model.py, which does the analogous job, also documents nf and device under Parameters and its return under Returns. Matching that is a small edit and keeps the API docs uniform.
| resolve_neighbor_graph_method("nv", torch.device("cpu")) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
These five cases pin the ladder, and I want to say they are the right shape -- patching availability and device rather than recomputing the cascade in the assertion is exactly what was missing from the equivalent test on #5912.
The gap is that they are the only new coverage, and they are pure resolver assertions: they never build a graph. The cross-builder numerical checks that do build one, test_vesin_matches_dense_energy_force and test_nv_matches_dense_energy_force further up this file, share the _eval helper, and it is single-frame:
coord = torch.tensor(rng.random((1, 6, 3)) * 4.0, ...)So parity between vesin and dense is established only at nf == 1 -- the regime the vesin builder's own docstring says it is "intended for" -- while this PR makes vesin the default precisely for the batched regime, where nothing compares it against dense. That matters slightly more than a generic coverage note because the graph lower accumulates with segment_sum over edges and edge ordering after canonicalize is an independent implementation per builder, so multi-frame agreement does not follow from single-frame agreement; it needs its own case.
A parametrization of _eval over nf in {1, 4} would cover it, and it is the test I would want in place before the default moves, whichever way the vesin question above is settled.
Closes #5902
Summary
resolve_auto_graph_builder(device)policy for the carry-all NeighborGraph builders: CUDA prefersnvthenvesinthendense; CPU prefersvesinthendense(asestays explicit-only).None/"auto") from hard-coded"dense"to that ladder, and use the same helper in DeepEval (default"auto") and compiled training's eager_forward_graph..pt2artifacts are unchanged.Why existing tests missed this
Builder dispatch already had vesin/nv vs dense energy/force parity, but nothing asserted that the model/DeepEval default would pick an O(N) builder when available, and compiled training still hardcoded dense while claiming to match the eager default-flip. The new resolver unit tests pin the availability ladder; the extended dispatch and DeepEval tests pin value-transparency of
None/"auto"against the resolved concrete builder.Validation
ruff check/ruff format --checkon touched filespytestfocused suite: 20 passed, 1 skipped (nvCUDA-only)source/tests/pt_expt/utils/test_neighbor_graph_method.pysource/tests/pt_expt/model/test_graph_builder_dispatch.py.pt2parityneighbor_graph_methodSummary by CodeRabbit
New Features
Bug Fixes
Tests